perf(ThemeProvider): Reduce unnecessary renders and effect cascades#7695
perf(ThemeProvider): Reduce unnecessary renders and effect cascades#7695mattcosta7 wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 8cb69d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
github-actions
bot
added
the
integration-tests: recommended
There was a problem hiding this comment.
Pull request overview
This PR refactors ThemeProvider implementations (both @primer/react and @primer/styled-react) to simplify SSR color-mode handoff handling by removing the ReactDOM-based flushSync workaround and by making context value construction more stable.
Changes:
- Replace the server color-mode passthrough
useRef+ReactDOM.flushSyncworkaround with aserverColorModestate that is cleared after hydration. - Memoize the
ThemeContext.Providervalue viauseMemoto reduce unnecessary context value churn. - Remove the unused
react-domimport from both ThemeProvider implementations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/styled-react/src/components/ThemeProvider.tsx | Aligns styled-react ThemeProvider SSR handoff logic with a state-based server passthrough and memoized context value. |
| packages/react/src/ThemeProvider.tsx | Updates core ThemeProvider SSR handoff logic to remove ReactDOM.flushSync usage and memoizes the context value. |
Comments suppressed due to low confidence (1)
packages/react/src/ThemeProvider.tsx:88
- ThemeProvider has an existing unit test suite, but the
preventSSRMismatch/ server handoff path introduced here isn’t covered. Please add a test that simulates server handoff data (e.g., by mockinguseIdto a stable value and inserting a matching__PRIMER_DATA_<id>__script element) and asserts that the provider initially uses the server-resolved color mode and then switches to client resolution after hydration.
// Lazy initializer reads DOM + parses JSON once instead of every render
const [serverColorMode, setServerColorMode] = React.useState<ColorMode | undefined>(
() => getServerHandoff(uniqueDataId).resolvedServerColorMode,
)
const [colorMode, setColorMode] = useSyncedState(props.colorMode ?? fallbackColorMode ?? defaultColorMode)
const [dayScheme, setDayScheme] = useSyncedState(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme)
const [nightScheme, setNightScheme] = useSyncedState(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme)
const systemColorMode = useSystemColorMode()
const resolvedColorMode = serverColorMode ?? resolveColorMode(colorMode, systemColorMode)
const colorScheme = chooseColorScheme(resolvedColorMode, dayScheme, nightScheme)
const {resolvedTheme, resolvedColorScheme} = React.useMemo(
() => applyColorScheme(theme, colorScheme),
[theme, colorScheme],
)
// After hydration, clear the server passthrough so client-side color mode takes over
React.useEffect(
function clearServerPassthrough() {
if (serverColorMode !== undefined) {
setServerColorMode(undefined)
}
},
[serverColorMode],
)
mattcosta7
changed the title
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/16765 |
|
Integration test results from github/github-ui:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
Summary
Performance improvements to
ThemeProviderin both@primer/reactand@primer/styled-react. All changes maintain identical user-facing behavior.Changes
1. Lazy
useStatefor SSR server handoffgetServerHandoff()(DOM read +JSON.parse) was called during every render. Now uses a lazyuseStateinitializer so it runs exactly once.2. Simplified SSR hydration effect
Before:
setTimeout→ReactDOM.flushSync(setColorMode(resolved))→setColorMode(original)— 3 renders, forced sync flush, temporarily corruptedcolorModestate.After:
setServerColorMode(undefined)in a mount effect — 1 state update, no cascading,colorModeis never touched.This also removes the
ReactDOMimport entirely.3. Memoized context value
The
ThemeContext.Providervalue was a new object literal every render, causing all consumers to re-render even when nothing changed. Now wrapped inuseMemo.4. Guarded
setSystemColorModeon mountuseSystemColorModecalledsetSystemColorModeunconditionally in its effect, even thoughuseState(getSystemColorMode)already read the correct value. Now uses a functional updater so React bails out when the value has not changed.Scenario analysis
Testing
All 49 existing ThemeProvider tests pass (20 in
@primer/react, 29 in@primer/styled-react).